-
Notifications
You must be signed in to change notification settings - Fork 84
fix: resolve strict mode incompatibilities with portals and popups (FE-7197) #7688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| export const TopModalProvider = ({ children }: { children: ReactNode }) => { | ||
| const [topModal, setTopModal] = useState<HTMLElement | null>(null); | ||
|
|
||
| // can't add the setter to the global list inside useEffect because that doesn't run until |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question(non-blocking): Are we all good to remove this ref and condition for first render? The comment states that we can't add the setter to the global list inside a useEffect and we're now doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be okay now, since Modal no longer attempts to access this global on initial render.
Portal now creates its attachment node on a separate render to when the portal content is displayed. Because of this change, Modal has been re-structured (see below) so its internal hooks, like useModalManager, are only called once the modal content is rendered.
// src/__internal__/modal/modal.component.tsx#L179
const Modal = (props: ModalProps) => (
<Portal>
{/* 👇 `Portal` only renders this once its attachment node is present */}
<ModalRoot {...props} />
</Portal>
);| const [portalNode, setPortalNode] = useState<HTMLElement | null>(null); | ||
| const [didCreateNode, setDidCreateNode] = useState(false); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking):
useEffect(() => {
// Find or create portal node
let node = document.getElementById(id);
const shouldCreateNode = !node;
if (shouldCreateNode) {
node = document.createElement("div");
node.setAttribute("id", id);
document.body.appendChild(node);
}
setDidCreateNode(shouldCreateNode);
setPortalNode(node);
// Cleanup portal node if component created it
return () => {
if (shouldCreateNode) {
node?.remove();
}
};
}, [id]);
| @@ -0,0 +1,22 @@ | |||
| import { useEffect, useState } from "react"; | |||
|
|
|||
| function canUseDOM() { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): we could use the getDocument and getWindow utils here instead
PortalContext was added to ensure Portal was captured by Chromatic properly, by attaching the React portal to Storybook's root element. Storybook no longer uses the referenced root, so this context is unused.
4f68f8d to
6e7cd4b
Compare
6e7cd4b to
0b8e605
Compare
62e315e
62e315e to
6f9cbda
Compare
6f9cbda to
688ea30
Compare
Proposed behaviour
For Carbon components involving React Portals, resolve inconsistencies in rendering behaviour:
Portal- when unmounting, properly clean up DOM node created as the container for the portal contentPopover- always render portal content in document body or nearest Carbon modal, to avoid the need to create and cleanup a DOM node.CarbonProvider- ensure global used in updating topmost modal is cleaned up properly.Current behaviour
Since #7148 was released, several Carbon components that use React Portals show unexpected behaviour when React Strict Mode is enabled, such as modals and popups not appearing at all. This occurs because these components have rendering inconsistencies that are surfaced by the stricter Strict Mode checks introduced in React v18.
Before upgrading Carbon to support newer React versions, we should ensure all components behave correctly under Strict Mode to prepare for future React changes.
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions
npm run start:strict-modeAdvancedColorPickerAlertConfirmDialogSidebarMenuFullscreenResponsiveVerticalMenuToastTooltipVerticalMenuFullscreen